Skip to content

[dotnet] Address nullable warnings#15389

Closed
RenderMichael wants to merge 1 commit intoSeleniumHQ:trunkfrom
RenderMichael:nullable-warnings
Closed

[dotnet] Address nullable warnings#15389
RenderMichael wants to merge 1 commit intoSeleniumHQ:trunkfrom
RenderMichael:nullable-warnings

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Mar 8, 2025 •
edited by qodo-code-review bot
Loading

User description

Description

This PR addresses lingering nullability warnings which can be fixed trivially.

Remaining work is contained in #15380 and #15379

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix

Description

  • Addressed nullable warnings in multiple Network classes.

  • Added null-forgiving operator (!) to RequestId assignments.

  • Updated WebDriver class to make NetworkManager nullable.

  • Improved code safety by handling potential nullability issues.

Changes walkthrough 📝

Relevant files
Bug fix
V131Network.cs
Fix nullable warnings in V131Network class                             

dotnet/src/webdriver/DevTools/v131/V131Network.cs

  • Added null-forgiving operator (!) to RequestId assignments.
  • Ensured null safety in ContinueRequest and AddResponseBody methods.
    		• +4/-4     
    V132Network.cs
    Fix nullable warnings in V132Network class                             

    dotnet/src/webdriver/DevTools/v132/V132Network.cs

  • Added null-forgiving operator (!) to RequestId assignments.
  • Ensured null safety in ContinueRequest and AddResponseBody methods.
    		• +4/-4     
    V133Network.cs
    Fix nullable warnings in V133Network class                             

    dotnet/src/webdriver/DevTools/v133/V133Network.cs

  • Added null-forgiving operator (!) to RequestId assignments.
  • Ensured null safety in ContinueRequest and AddResponseBody methods.
    		• +4/-4     
    V85Network.cs
    Fix nullable warnings in V85Network class                               

    dotnet/src/webdriver/DevTools/v85/V85Network.cs

  • Added null-forgiving operator (!) to RequestId assignments.
  • Ensured null safety in ContinueRequest and AddResponseBody methods.
    		• +4/-4     
    WebDriver.cs
    Update WebDriver class for nullable `NetworkManager`         

    dotnet/src/webdriver/WebDriver.cs

  • Made NetworkManager field nullable.
  • Improved nullability handling for internal fields.
    		• +1/-1     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Mar 8, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Null Check

    The AddResponseBody method doesn't check if bodyResponse is null before accessing its properties, unlike the equivalent methods in other version files which have this check.

     
    var bodyResponse = await fetch.GetResponseBody(new Fetch.GetResponseBodyCommandSettings() { RequestId = responseData.RequestId! }).ConfigureAwait(false);
if (bodyResponse.Base64Encoded)
{
    responseData.Content = new HttpResponseContent(Convert.FromBase64String(bodyResponse.Body));
}

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Mar 8, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add null check

    The code is missing a null check for bodyResponse before accessing its
    properties. This could lead to a NullReferenceException if the response from
    fetch.GetResponseBody() is null.

    dotnet/src/webdriver/DevTools/v85/V85Network.cs [282-296]

     public override async Task AddResponseBody(HttpResponseData responseData)
 {
     if (responseData is null)
     {
         throw new ArgumentNullException(nameof(responseData));
     }
 
     // If the response is a redirect, retrieving the body will throw an error in CDP.
     if (responseData.StatusCode < 300 || responseData.StatusCode > 399)
     {
         var bodyResponse = await fetch.GetResponseBody(new Fetch.GetResponseBodyCommandSettings() { RequestId = responseData.RequestId! }).ConfigureAwait(false);
-        if (bodyResponse.Base64Encoded)
+        if (bodyResponse != null && bodyResponse.Base64Encoded)
         {
             responseData.Content = new HttpResponseContent(Convert.FromBase64String(bodyResponse.Body));
         }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies a potential NullReferenceException in the V85Network.cs file. Unlike the other version files (v131-v133) which already have a null check for bodyResponse, the v85 implementation is missing this check before accessing the Base64Encoded property, which could cause runtime errors.

    		Medium
    Learned
    best practice
    Replace null-forgiving operators with proper null validation to ensure runtime safety

    The PR adds null-forgiving operators (!) to RequestId properties, which
    suppresses compiler warnings but doesn't provide runtime safety. Instead, use
    null-conditional operators (?.) with null-coalescing operators (??) to handle
    potential null values safely. This provides both compile-time and runtime
    protection against null reference exceptions.

    dotnet/src/webdriver/DevTools/v131/V131Network.cs [235]

    -await fetch.ContinueRequest(new ContinueRequestCommandSettings() { RequestId = requestData.RequestId! }).ConfigureAwait(false);
+var requestId = requestData.RequestId ?? throw new ArgumentException("RequestId cannot be null", nameof(requestData));
+await fetch.ContinueRequest(new ContinueRequestCommandSettings() { RequestId = requestId }).ConfigureAwait(false);
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    		Low
    • More

    nvborisenko
    nvborisenko reviewed Mar 9, 2025
    View reviewed changes
    dotnet/src/webdriver/DevTools/v131/V131Network.cs
    var commandSettings = new FulfillRequestCommandSettings()
    {
    RequestId = requestData.RequestId,
    RequestId = requestData.RequestId!,
    Copy link
    Member

    @nvborisenko nvborisenko Mar 9, 2025

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    At first glance requestData.RequestId should not be nullable?

    Copy link
    Contributor Author

    @RenderMichael RenderMichael Mar 9, 2025

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    RequestId can be null in one specific circumstance: when a user creates an HttpRequestData themselves.

    This only happens in one place, and it can even cause problems. I filed an issue about it: #15380

    However, these HttpRequestDatas are known to be created internally, so we know RequestId is not null.

    Copy link
    Member

    @nvborisenko nvborisenko Mar 9, 2025

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Good, so requestData.RequestId should not be null. Then I propose to not suppress this in this PR, because of we will definitely forget re-suppress it. Let's try to fix root cause, even if it will be a breaking change.

    What I understand, we have 2 ways:

    • Slightly adjust public API to require RequestID to be not nullable (following standard policy of deprecation), will be a part of v4
    • Entirely revisit public API of NetworkManager in v5 (how this API looks, how it could look, including renaming StartMonitoring to StartMonitoringAsync)

    @RenderMichael if can manage the 1st way easily with minimal effort, it would be great (as short-term solution before v5).

    Copy link
    Contributor Author

    @RenderMichael RenderMichael Mar 9, 2025

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    That sounds like a much better solution.

    All 3 proposed solutions fix the issue (2 is my favorite but breaking, 3 is still good, 1 is the backup which is not a breaking change).

    @RenderMichael RenderMichael marked this pull request as draft March 14, 2025 19:32
    @github-actions github-actions bot added the J-stale Applied to issues that become stale, and eventually closed. label Sep 10, 2025
    @github-actions github-actions bot closed this Sep 25, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @nvborisenko nvborisenko nvborisenko left review comments

    Assignees

    No one assigned

    Labels

    J-stale Applied to issues that become stale, and eventually closed. Review effort 1/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @RenderMichael @nvborisenko